Home Page Prep #37

jtcaovan · 2024-06-13T20:15:59Z

Pull Request Template

Issue Overview

This PR addresses #168

This change addresses the issue in full
This change addresses only certain aspects of the issue
This change is a dependency for another issue
This change has a dependency from another issue

Description

This PR removes the previous sheltertech homepage and replaces it with our new components. Changes are made across all components to account for change in data fetching and styling.

Hero
Category Cards
Opportunity Cards
Event Cards
Two Column Content

Note: Newsletter component is still needed

How Can This Be Tested/Reviewed?

Run local env and view home page. Should match up to Figma designs here. There are a lot of diffs since I had to replace the previous home page, move all of the data fetching into the home page, and style each component to fit. Would probably be best to view home page first for visual issues then review code quality after

Instead of fetching the data in each separate component, the data is now all fetched on the home page
Wraps home page components with HomePageSection - content should have a max width and background color should expand full width
Fixes issue with Two Column Content image disappearing on smaller breakpoints
Adds focus/active states to opp/event cards
Review code quality
Editing the home page fields in Sanity should immediately update the home page
https://our415.sanity.studio/development/structure/homePage;293c2d9c-0bba-4335-9532-3937f6dc9d86

Checklist:

Reviewer Notes:

Steps to review a PR:

Read and understand the issue, and ensure the author has added QA notes
Review the code itself from a style point of view
Pull the changes down locally and test that the acceptance criteria is met
Also review the acceptance criteria on the Netlify deploy preview (noting that these do not yet include any backend changes made in the PR)
Either explicitly ask a clarifying question, request changes, or approve the PR if there are small remaining changes but the PR is otherwise good to go

On Merge:

If you have one commit and message, squash. If you need each message to be applied, rebase and merge.

rosschapman · 2024-06-21T02:39:15Z

app/pages/HomePage/HomePage.tsx

+  categoriesSection: FeaturedCategoriesData;
+  opportunitySection: OppEventCardData;
+  eventSection: OppEventCardData;
+  /* Fix naming in Sanity schema */


I like the idea of putting this comment here but I don't understand it without context. Can you please add a couple more sentences to explain what you are fixing?

rosschapman · 2024-06-21T02:40:54Z

app/pages/HomePage/HomePage.tsx

+      />
+      <OppEventCardSection sectionType="event" sectionData={eventSection} />
+      <TwoColumnContentSection {...twoColumnContentData} />
+      {/* Newsletter Component */}


Is there a ticket in Notion you could reference here in case someone else has to pick up the work?

Newsletter signup

rosschapman · 2024-06-21T02:41:43Z

app/styles/st-base/_forms.scss

+// button {
+//   @extend %font-size-default;
+//   border: 0;
+//   // height: calc-em(44px);
+//   padding: calc-em(6px) calc-em(20px);
+//   color: $color-white;
+//   background: $color-green;
+//   display: inline-block;
+//   font-weight: 600;
+//   &:active,
+//   &:hover,
+//   &:focus {
+//     box-shadow: 0px 2px 4px 0px rgba(0, 0, 0, 0.2);
+//   }

-  &.danger {
-    background: $color-red;
-  }
-}
+//   &.danger {
+//     background: $color-red;
+//   }
+// }


Keep or 🧹 ?

rosschapman · 2024-06-21T02:43:48Z

app/pages/HomePage/HomePage.tsx

+// Remove when new categories is created
+// other components are dependent on this list


Suggestion: consider adding a TODO or similar label so this isn't mistaken with explaining behavior. IME these comments sometimes accidentally stick around and become esoteric erratum to be deciphered by future archeologists under candlelight.

rosschapman · 2024-06-21T02:48:02Z

app/pages/AboutPageOur415/AboutPage.tsx

-        (section: JSX.IntrinsicAttributes & TwoColumnContentSection) => {
-          return <TwoColumnContentSection key={section._id} {...section} />;
+        (section: JSX.IntrinsicAttributes & TwoColumnContent) => {
+          return <TwoColumnContentSection key={section.key} {...section} />;


I'm curious why you change to key here. I also don't see it on any type declaration in this PR (though admittedly I'm ignorant of the codebase and would believe it's somewhere else.)

Thanks for catching that, I reverted back to using _id here

rosschapman · 2024-06-21T02:50:48Z

app/pages/HomePage/HomePage.tsx

+    fetchHomePageData();
+  }, []);
+
+  if (!homePageData) {


Suggestion: do an exact null check here to be clear about runtime expectations matching the type. Otherwise I would second guess that the value of homePageData could be a falsy value other than null.

Suggested change

if (!homePageData) {

if (homePageData === null) {

rosschapman · 2024-06-21T02:55:42Z

app/pages/HomePage/HomePage.tsx

+    }
+      }`;
+
+      const result: HomePageData = await client.fetch(query);


Are we punting on error handling for the time being? Or the Sanity API returning a structurally correct type but with empty data? That's ok, I'm just curious because if Sanity sent something back unexpected I wonder about the consequences.

Added in error handling!

rosschapman

Nice work @jtcaovan! I think I get the gist of what's going on here. Normally when getting to know a codebase I'd spend some time with you synchronously to talk more about it. But I know y'all are cooking toward the deadline. All seems reasonable to me. No pressure to resolve my questions before merging.

rosschapman · 2024-06-21T03:07:45Z

app/pages/HomePage/components/Section/Section.tsx

  children,
 }: {
-  title: string;
+  title?: string;


Adding this optionality seems like good correction!

rosschapman · 2024-06-21T03:10:42Z

app/components/ui/Section/CategorySection.tsx

+
+  const { header, subheader, backgroundColor, featuredCategoriesSection } =
+    sectionData;
+  const categories = featuredCategoriesSection[0].category;


Huh, strange asymmetry between categories and category.

jtcaovan temporarily deployed to dev June 13, 2024 20:16 — with GitHub Actions Inactive

jtcaovan temporarily deployed to dev June 18, 2024 01:24 — with GitHub Actions Inactive

jtcaovan temporarily deployed to dev June 18, 2024 01:32 — with GitHub Actions Inactive

jtcaovan added the wip label Jun 18, 2024

jtcaovan temporarily deployed to dev June 18, 2024 01:38 — with GitHub Actions Inactive

jtcaovan temporarily deployed to dev June 18, 2024 23:55 — with GitHub Actions Inactive

jtcaovan marked this pull request as ready for review June 18, 2024 23:57

This was referenced Jun 19, 2024

Category Cards Old #35

Closed

Category Cards #43

Closed

Category cards #44

Merged

jtcaovan temporarily deployed to dev June 19, 2024 22:40 — with GitHub Actions Inactive

jtcaovan temporarily deployed to dev June 19, 2024 23:13 — with GitHub Actions Inactive

jtcaovan temporarily deployed to dev June 19, 2024 23:17 — with GitHub Actions Inactive

jtcaovan added 17 commits June 20, 2024 09:51

initial home page prep

2458f1e

move and export type from two column component

7e75cb8

create 2 column component for home page

1ad45f1

rm resource count state

b596294

refactor into one api call from sanity

bb7a42f

connect oppevent cards to sanity data

19564ca

fix button issue with two column content, misc changes

2bfb53d

refactor home page section component

63eef68

add focus/active state on event cards

c8d2fe4

refactor event section to use home page section

32a4e1a

fix small breakpoint issue with two column section

61b8175

fix prettier

71efed6

fix build error

d94ecb8

fix build errors again

2470d9c

add in category cards

d56197a

fetch home page 2 column content

db63bdd

fix slug type

2e325c0

jtcaovan added 3 commits June 20, 2024 12:03

rm console.log

962927d

fix two column types

c893886

fix prettier/lint issues

3f4366e

jtcaovan force-pushed the home-page-prep branch from 082fce2 to 3f4366e Compare June 20, 2024 19:16

jtcaovan temporarily deployed to dev June 20, 2024 19:16 — with GitHub Actions Inactive

jtcaovan added needs review and removed wip labels Jun 20, 2024

refactor see all services icon

dc3aaef

jtcaovan temporarily deployed to dev June 20, 2024 19:29 — with GitHub Actions Inactive

jtcaovan requested review from rosschapman, kramduckner and adriencyberspace June 20, 2024 19:35

rosschapman reviewed Jun 21, 2024

View reviewed changes

rosschapman approved these changes Jun 21, 2024

View reviewed changes

rosschapman reviewed Jun 21, 2024

View reviewed changes

jtcaovan added 3 commits June 21, 2024 08:17

update comments, address pr review comments

9d789f8

add basic error handling

72ad471

fix no console lint error, revert two column prop type

0a8b8a7

jtcaovan temporarily deployed to dev June 21, 2024 15:37 — with GitHub Actions Inactive

rm previous global button styling

9660204

jtcaovan temporarily deployed to dev June 21, 2024 15:38 — with GitHub Actions Inactive

jtcaovan merged commit 7fb276c into development Jun 21, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Home Page Prep #37

Home Page Prep #37

jtcaovan commented Jun 13, 2024 •

edited

Loading

rosschapman Jun 21, 2024

rosschapman Jun 21, 2024

jtcaovan Jun 21, 2024

rosschapman Jun 21, 2024

jtcaovan Jun 21, 2024

rosschapman Jun 21, 2024 •

edited

Loading

rosschapman Jun 21, 2024

jtcaovan Jun 21, 2024

rosschapman Jun 21, 2024 •

edited

Loading

rosschapman Jun 21, 2024 •

edited

Loading

jtcaovan Jun 21, 2024

rosschapman left a comment

rosschapman Jun 21, 2024

rosschapman Jun 21, 2024

		// Remove when new categories is created
		// other components are dependent on this list

Home Page Prep #37

Home Page Prep #37

Conversation

jtcaovan commented Jun 13, 2024 • edited Loading

Pull Request Template

Issue Overview

Description

How Can This Be Tested/Reviewed?

Checklist:

Reviewer Notes:

On Merge:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosschapman Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosschapman Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

rosschapman Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosschapman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcaovan commented Jun 13, 2024 •

edited

Loading

rosschapman Jun 21, 2024 •

edited

Loading

rosschapman Jun 21, 2024 •

edited

Loading

rosschapman Jun 21, 2024 •

edited

Loading